Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid calling FamilyPcgs #2656

Merged
merged 1 commit into from
Aug 14, 2023
Merged

avoid calling FamilyPcgs #2656

merged 1 commit into from
Aug 14, 2023

Conversation

ThomasBreuer
Copy link
Member

Calling GAP's FamilyPcgs with a proper subgroup of a pc group yields a pcgs for the full pc group, see gap-system/gap/issues/5483.

If I remember right then the idea of relations(G) for a group G was that the relations hold w.r.t. the generators stored for G, and then the proposed change will in general violate this assumption.

Perhaps it would be safer to change relations such that also the list of generators is returned w.r.t. which the relations hold.

(Probably this will not be a complete fix.)
@fieker
Copy link
Contributor

fieker commented Aug 11, 2023

I need the relations relative to exactly the generators of the group, getting differnt ones will not be helpful

@ThomasBreuer
Copy link
Member Author

@fieker Then what about the following: Oscar.relations(G::PcGroup) checks whether the stored generators actually are a pcgs. If yes then it calls GAP's IsomorphismFpGroupByPcgs, otherwise it calls GAP's IsomorphismFpGroupByGenerators.
(In fact, one could then do this in Oscar.relations(G::Oscar.GAPGroup), and no special method for pc groups is needed.)

@fieker
Copy link
Contributor

fieker commented Aug 11, 2023 via email

@ThomasBreuer
Copy link
Member Author

Suppose the given group G is a pc group but the stored generators are not a pcgs for G.
Then we want relations w.r.t. the stored generators, thus we cannot call IsomorphismFpGroupByPcgs, and we will not get pc relations.

If the stored generators are a pcgs for G and if IsomorphismFpGroupByPcgs is called with this pcgs then it returns the pc relations w.r.t. this pcgs.

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #2656 (4a55be3) into master (c66d764) will increase coverage by 0.00%.
Report is 4 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2656   +/-   ##
=======================================
  Coverage   71.91%   71.92%           
=======================================
  Files         429      429           
  Lines       60812    60812           
=======================================
+ Hits        43731    43736    +5     
+ Misses      17081    17076    -5     
Files Changed Coverage Δ
experimental/GModule/Cohomology.jl 50.43% <0.00%> (ø)

... and 3 files with indirect coverage changes

@fingolfin fingolfin merged commit 39d2ffd into oscar-system:master Aug 14, 2023
12 of 15 checks passed
fingolfin pushed a commit that referenced this pull request Aug 15, 2023
@ThomasBreuer ThomasBreuer deleted the TB_FamilyPcgs branch August 16, 2023 11:48
HechtiDerLachs pushed a commit to HechtiDerLachs/Oscar.jl that referenced this pull request Aug 17, 2023
(Probably this will not be a complete fix.)
justus-springer pushed a commit to justus-springer/Oscar.jl that referenced this pull request Aug 18, 2023
(Probably this will not be a complete fix.)
justus-springer pushed a commit to justus-springer/Oscar.jl that referenced this pull request Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants